Implement play queue sync for Subsonic #1241

mattwellss · 2025-08-10T18:17:54Z

Store user play queue in oc_preferences
support for music tracks and podcast episodes
resolves Support synced play queue #1103
builds on Subsonic: Implement queue syncing #1104 (credit to original author jplitza)

Original pull request includes discussion of limiting the response size of getPlayQueue, which isn't implemented at this point, but it's worth discussing

paulijar · 2025-08-10T19:43:39Z

Thanks. Looks good on the first glance. I'll do some testing later once I have time.

paulijar · 2025-08-10T19:49:16Z

lib/Controller/SubsonicController.php

+		if (isset($current))
+			$playQueue['current'] = $current;
+		if (isset($position))
+			$playQueue['position'] = $position;


It's a good habit to always use curly braces on your if statements. For one famous example of what might happen if you don't, see goto fail.

mattwellss · 2025-08-10T20:35:33Z

lib/Controller/SubsonicController.php

+		foreach ($playQueue['entry'] as &$entry) {
+			[$type, $id] = self::parseEntityId($entry);
+			if ($type === 'track') {
+				$entry = $this->trackToApi(
+					$this->trackBusinessLayer->find($id, $this->user())
+				);
+			} else if ($type === 'podcast_episode') {
+				$entry = $this->podcastEpisodeBusinessLayer->find($id, $this->user())->toSubsonicApi();
+			}
+		}


I wonder about this section. If an $entry somehow doesn't match either "track" or "podcast_episode", it'll be left in the response as an invalid element. Are there any other types of media that might end up saved in savePlayQueue? If this is a reasonable concern (and I dunno if it is), I can solve it by sanitizing the input on savePlayQueue or by generating a new array of just the valid items from $playQueue['entry'] in getPlayQueue

That's a good question. The third kind of media playable on the Subsonic clients is a radio station. I'm not certain if any client puts those to the play queue, though. The API would allow also video playback, but this is not supported in the Music app (at least for now or in the near future).

But yeah, it would probably be safer to construct the list so that only the supported types of entities would be retained.

paulijar · 2025-08-17T17:08:42Z

Sorry for the delay, I have been busy with other things but now I managed to do some testing with this.

Functionally, I think this works pretty much as it should. There's one minor deviation from the Subsonic XML schema https://www.subsonic.org/pages/inc/api/schema/subsonic-rest-api-1.16.1.xsd as there the attribute username is defined as required but it's missing from the current implementation here. I would guess that no client actually uses this attribute for anything but better to follow the schema, still.

However, there's also one major issue now and that's the performance. The execution time of getPlayQueue now grows linearly with the number of tracks on the queue. For example, with 100 tracks in the queue, I saw execution time to be 25 seconds or more which is way too much. The reason for this is most certainly that there is call to $this->trackToApi for each track of the queue. This will cause several SQL queries to be executed per each track and all that adds up quickly.

The good news is that this is a kind of problem which has been already solved elsewhere in the Subsonic API. There exists the function SubsonicController::tracksToApi which does the conversion for multiple tracks at once, using clever tricks to minimize the number of SQL queries. So the function getPlayQueue should now be reorganized so that all the Track objects would be fetched first from TrackBusinessLayer and then all of them would be converted at once to the Subsonic API format. What makes this non-trivial is the fact that there may be podcast episodes interlaced among the songs. So likely, we would need to construct the result in multiple steps. It could be something like this:

$entries = \array_map(fn($entry) => self::parseEntityId($entry), $playQueue['entry']);
$trackEntries = \array_filter($entries, fn($entry) => $entry[0] === 'track');
$trackIds = \array_map(fn($entry) => $entry[1], $trackEntries);
$tracks = $this->trackBusinessLayer->findById($trackIds, $this->user());
$tracks = ArrayUtil::createIdLookupTable($tracks); // track IDs will become keys
$apiTracks = $this->tracksToApi($tracks); // this preserves the keys of the array

$entries = \array_map(function($entry) {
	[$type, $id] = $entry;
	if ($type === 'track') {
		return $apiTracks[$id] ?? null;
	} else if ($type === 'podcast_episode') {
		return $this->podcastEpisodeBusinessLayer->find($id, $this->user())->toSubsonicApi();
	} else {
		return null;
	}
}, $entries);
$entries = \array_filter($entries);

Caveat: I didn't try to run this, syntax errors are possible.

While at it, one thing to consider is what will happen in case the queue contains an ID which doesn't match any item in the library. This could happen for example if the item has been deleted since the queue was saved. At the moment, the action getPlayQueue will fail with the Subsonic error "entity not found". This will happen when BusinessLayer::find throws a BusinessLayerException and it's not caught within SubsonicController. It could be nicer if only the invalid entity was omitted but other parts of the queue would be returned. Of course, the case shouldn't be terribly common so the problem is quite minor.

Edit: I made another optimization on the example code above. Also the call to BusinessLayer::find makes one SQL query per each call. But it can be replaced with (a bit badly named) BusinessLayer::findById which allows finding many entities with a single query. The same optimization could still be made also on the podcast episodes.

Edit2: Using findById actually solves also the point about IDs which do not match any entities: This function will not throw but simply omit any items which were not found.

mattwellss · 2025-08-17T18:21:56Z

Thanks for the review! I appreciate the testing/feedback and I'll work on those improvements. Unsure if I'll get more commits pushed until next weekend, though

mattwellss · 2025-08-17T18:54:33Z

Edit: I made another optimization on the example code above. Also the call to BusinessLayer::find makes one SQL query per each call. But it can be replaced with (a bit badly named) Business::findById which allows finding many entities with a single query. The same optimization could still be made also on the podcast episodes.

Funny! I was just looking at the suggestion and wondering if bulk track find functionality existed :)

mattwellss · 2025-08-20T12:25:13Z

lib/Controller/SubsonicController.php

+		$playQueue = json_decode($this->configManager->getUserValue($this->user(), $this->appName, 'play_queue', 'false'), true);
+
+		if (!$playQueue) {
+			return $this->subsonicResponse([]);


I'm returning an empty response rather than an empty playqueue when we haven't saved one. Seems better than generating metadata for a canned response, and it follows the API better: https://subsonic.org/pages/api.jsp#getPlayQueue

Returns a <subsonic-response> element with a nested <playQueue> element on success, or an empty <subsonic-response> if no play queue has been saved.

mattwellss · 2025-08-20T12:38:05Z

lib/Controller/SubsonicController.php

+					$pqEntries[] = $apiTracks[$id];
+				} else if ($type === 'podcast_episode') {
+					try {
+						$pqEntries[] = $this->podcastEpisodeBusinessLayer->find($id, $this->user())->toSubsonicApi();


similar to looking up tracks individually, many podcast episodes in a play queue could presumably cause a slow response. Should we include bulk episode find a la bulk track find as part of this work?

Yes, I think that would be good

paulijar · 2025-08-20T17:24:33Z

lib/Controller/SubsonicController.php

+			'current' => $current,
+			/** @see Util::formatZuluDateTime (if only we could pass a datetime!) */
+			'changed' => $changedDateTime->format('Y-m-d\TH:i:s.v\Z'),
+			'username' => $u


We should use $this->user() here instead of the URL argument $u. The reason is that the Music app supports the OpenSubsonic extension API Key Authentication which means that the client may pass the argument apiKey instead of u and p. The function will return the correct username in either case.

Another small difference is that the URL argument $u is case insensitive but $this->user() always contains the "proper" casing i.e. the same format in which the username was given when the account was created.

paulijar · 2025-08-21T20:29:01Z

Performance-wise, the latest version works great with local songs: While the 100-song queue previously took 25+ seconds, now I can get a 500-song queue in less than a second. 👍

mattwellss · 2025-08-21T21:27:06Z

lib/Controller/SubsonicController.php

+		$typeLookupMap = [
+			'track' => [
+				'lookupFn' => [$this->trackBusinessLayer, 'findById'],
+				'toApiFn' => [$this, 'trackstoApi']
+			],
+			'podcast_episode' => [
+				'lookupFn' => [$this->podcastEpisodeBusinessLayer, 'findById'],
+				'toApiFn' => [$this, 'podcastEpisodestoApi']
+			]
+		];
+
+		/** @var array{'track': Track[], 'podcast_episode': PodcastEpisode[]} $apiEntries */
+		$apiEntries = array_merge([], ...array_map(
+			fn ($type, $methods) => [$type => $methods['toApiFn'](
+				ArrayUtil::createIdLookupTable($methods['lookupFn'](
+					\array_map(
+						fn ($entry) => $entry[1],
+						\array_filter($parsedEntries, fn ($parsedEntry) => $parsedEntry[0] === $type)
+					),
+					$this->user()
+				))
+			)],
+			array_keys($typeLookupMap),
+			array_values($typeLookupMap)
+		));


PHP doesn't necessarily lend itself super great to this sort of programming, so I totally get it if you want to see intermediate vars introduced here.

I appreciate the functional programming style but it is true that this is a bit difficult to read or at least requires a lot of focusing to understand it correctly. As a rule of thumb, having more than two level of nesting in control structures is a bit of a code smell, and the same could probably be applied on these array_* functions which are essentially just higher-level control structures. Sometimes the deep nesting might be the best option but at least it's a sing that considering also other design options could be fruitful.

So if you can make it any easier to read with intermediate vars, then that's good. But if not, then that's not a blocker for merging. I'll give this another test round on Sunday or next week. If no problems are found, I'll merge this, unless you want to still change something.

I'm planning to release Music v2.3.0 rather soon, and I'd like to get this merged before that. The deadline for the release is the end of September, before NC32, but preferably I'd like to do it during the next two weeks.

Sorry to chime in, I'm just mostly agreeing and feel free to ignore me, I understand the release is more important, and I don't want to start a long discussion. I don't think PHP gets functional programming right, which you might already know. The best use of FP makes code also declarative and therefore easy to read. However, in PHP, array_map puts the function parameter first, which which poses multiple problems like being inconsistent with array_filter, for example. This makes me want to use a better functional library instead, like Pipeline, but it's just an example, it may not be appropriate to use in a Nextcloud App, and the function chaining may limit you. But at least Pipeline starts the logic with the first thing to do, so you don't read function calls backwards. Also, array_reduce, just like in other languages, is more of a primitive higher-order function, and therefore not so readable; usually, if you can use something better, you should. (Of course this is situational, and may not always be applicable.) I think if the code doesn't mix high- and low-level logic and stays declarative, then it doesn't necessarily harm code readability when you use multiple levels of nesting. It's just that other languages do it better, and more concisely.

Thanks for the feedback. Really looking forward to a new release and using this with my real library. I made an effort to create a few steps rather than requiring the reader's brain to hold all the transformations. If it's not better, at least it's a little different :D

It's good enough for now. If it starts bothering me, then maybe I'll reconsider the design later.

Anyway, I now tested the latest version and it seemed to run without any issues. Even Dsub worked now without any hick-ups with 500-item queues; maybe the problems I saw in it handling the long queues with the previous play-queue-PR was due to some of the API-noncompliances of that implementation 🤷.

The extra attributes "position" and "current" (track) are saved as JSON comment of the playlist.

- only tracks and podcast episodes allowed - add changedBy, position, and current, then array_filter nulls out - inline formatted changed datetime string

position can be an int, and array_filter without a callback is loosely typed. Scrutinizer found this in a previous commit but misses it here https://scrutinizer-ci.com/g/owncloud/music/inspections/d54ddd52-3e3c-4c11-9111-0ea3911c14a6/issues/files/lib/Controller/SubsonicController.php?status=new&orderField=path&order=asc&honorSelectedPaths=0

refactor playqueue['entry'] generation

- $type is now a sibling of functions in handler array - generate intermediate var for type's entity ids - generate intermediate var for type's entity instances - switch from array_reduce to array_filter'd array_map

…nctions This mechanism enabling a bit more concise expression has been introduced after the first PR for Subsonic play queue support was started.

scrutinizer-notifier · 2025-08-24T20:06:09Z

A new inspection was created.

paulijar · 2025-08-31T18:27:51Z

This is now released in Music v2.3.0.

paulijar reviewed Aug 10, 2025

View reviewed changes

mattwellss commented Aug 10, 2025

View reviewed changes

paulijar mentioned this pull request Aug 10, 2025

Subsonic: Implement queue syncing #1104

Closed

mattwellss commented Aug 20, 2025

View reviewed changes

paulijar reviewed Aug 20, 2025

View reviewed changes

mattwellss commented Aug 21, 2025

View reviewed changes

jplitza and others added 18 commits August 24, 2025 22:41

Subsonic API: Implement queue sync via "Queue" playlist

0b4ac76

The extra attributes "position" and "current" (track) are saved as JSON comment of the playlist.

Update play queue storage to use config manager

a78f15a

Support podcasts in play queue

37ba697

spaces -> tabs cleanup

46fc10b

add braces to control structures

bff4d78

clean up play queue generation

cdee455

- only tracks and podcast episodes allowed - add changedBy, position, and current, then array_filter nulls out - inline formatted changed datetime string

optimize getPlayQueue to use bulk track lookup

8983414

don't fail when podcast episode isn't found

80d0884

Return an empty response when no playqueue exists

4b0ca5b

Fix indentation

d59f34e

When saving a playqueue, require $u and $c; save $u in playqueue

22b635c

move note referring to time formatting

fdbcaab

switch from user param to user method

079cf13

Lookup use findById for bulk podcast lookup

07dc12d

refactor playqueue['entry'] generation

Refactor api entry generation

3baafda

- $type is now a sibling of functions in handler array - generate intermediate var for type's entity ids - generate intermediate var for type's entity instances - switch from array_reduce to array_filter'd array_map

Use automatic array to SubsoniResponse wrapping also on play queue fu…

1ea2676

…nctions This mechanism enabling a bit more concise expression has been introduced after the first PR for Subsonic play queue support was started.

Update changelog

cfa7048

paulijar force-pushed the feature/1103_queue-sync branch from c14efb5 to cfa7048 Compare August 24, 2025 20:05

paulijar merged commit 83d572c into owncloud:master Aug 24, 2025

mattwellss deleted the feature/1103_queue-sync branch August 24, 2025 20:12

Implement play queue sync for Subsonic #1241

Implement play queue sync for Subsonic #1241

Conversation

mattwellss commented Aug 10, 2025

Uh oh!

paulijar commented Aug 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulijar commented Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattwellss commented Aug 17, 2025

Uh oh!

mattwellss commented Aug 17, 2025

Uh oh!

mattwellss Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulijar commented Aug 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scrutinizer-notifier commented Aug 24, 2025

Uh oh!

paulijar commented Aug 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

paulijar commented Aug 17, 2025 •

edited

Loading

mattwellss Aug 20, 2025 •

edited

Loading